Add complexity param #430
Add complexity param #430
Conversation
resources/tests.mjs
Outdated
| @@ -66,7 +71,8 @@ Suites.push({ | |||
| page.getLocalStorage().getItem("javascript-es5"); | |||
| }, | |||
| tests: [ | |||
| new BenchmarkTestStep(`Adding${numberOfItemsToAdd}Items`, (page) => { | |||
| new BenchmarkTestStep(`Adding${getNumberOfItemsToAdd()}Items`, (page) => { | |||
There was a problem hiding this comment.
Maybe I should just stick to using defaultNumberOfItemsToAdd here, so that the metric names don't change?
There was a problem hiding this comment.
I think the name should reflect the complexity level, in an effort to avoid comparing a simple test run with a complex test run
There was a problem hiding this comment.
Maybe we could remove the number here and handle this during reporting time - update the code that consumes the name after the run when reporting in the UI or JSON as either TodoMVC-LocalStorage (Complexity 1.5) -> AddingItems, or TodoMVC-LocalStorage -> AddingItems (Complexity 1.5) when complexity !== 1? One tradeoff there is it would report with complexity even in tests that aren't complexity-aware, but that seems maybe fine given that this is an opt-in thing that you'd only use if you know what you're doing.
@rniwa WDYT?
There was a problem hiding this comment.
Definitely open to either suggestion. getNumberOfItemsToAdd() would work partially if the complexity param is passed-in via url-params only (but that's a bit brittle).
There was a problem hiding this comment.
Naively I'd do this:
- add the complexity value somewhere in the json report, but not in all tests. Also display it somewhere in the results interface.
- remove any mention of actual counts in the test titles
But I'm not convinced this is right, because we might want the tests themselves to hold this value so that it's clear in reports that we're not at the default complexity.
Also if we want to change the actual titles, I'd like to see the complexity in the test names more than in the steps.
There was a problem hiding this comment.
- I can add a
non-standard-paramswarning in the detailed results page to surface what's happening - I'd vouch for dropping mentioning the complexity params (much like other score-influencing params, we don't surface them anywhere else)
There was a problem hiding this comment.
In the latest commit I've just removed the reference to 100 in the step title.
|
Without looking closely at the code change, I think this is a neat idea and I could imagine us increasing the complexity of charts, text editing, etc if we move forward with it. Will be curious for feedback from @julienw. |
| @@ -101,11 +102,21 @@ function createUIForSyncStepDelay() { | |||
| return label; | |||
| } | |||
|
|
|||
| function createTimeRangeUI(labelText, initialValue, unit = "ms", min = 0, max = 1000) { | |||
| function createUIForComplexity() { | |||
| const { range, label } = createTimeRangeUI("Relative complexity", params.complexity, "x", 0, 10, 0.01); | |||
There was a problem hiding this comment.
I was thinking that rather than a float, maybe this can be an integer between 1 and 100, with 50 being the default?
Then later you can divide this number by 50 to get the multiplier.
And you can cut down a lot of the changes.
But this is merely a suggestion, I'm also happy with what you did.
There was a problem hiding this comment.
I kinda prefer a normalized float value, aiming quite literaly at a complexity-factor, 2x => it should run roughly twice as slow.
resources/tests.mjs
Outdated
| @@ -66,7 +71,8 @@ Suites.push({ | |||
| page.getLocalStorage().getItem("javascript-es5"); | |||
| }, | |||
| tests: [ | |||
| new BenchmarkTestStep(`Adding${numberOfItemsToAdd}Items`, (page) => { | |||
| new BenchmarkTestStep(`Adding${getNumberOfItemsToAdd()}Items`, (page) => { | |||
There was a problem hiding this comment.
Naively I'd do this:
- add the complexity value somewhere in the json report, but not in all tests. Also display it somewhere in the results interface.
- remove any mention of actual counts in the test titles
But I'm not convinced this is right, because we might want the tests themselves to hold this value so that it's clear in reports that we're not at the default complexity.
Also if we want to change the actual titles, I'd like to see the complexity in the test names more than in the steps.
|
Work in progress proposal for adding a variable complexity param.
complexity === 1.0: workloads run in default configurationcomplexity > 1.0: ideally workloads run proportionally slowercomplexity < 1.0: ideally workloads run proportionally fastBackground:
We're occasionally seeing issues with gc timing / optimisations heuristics with the current workloads. If we can dynamically change the duration / complexity of workloads it's a bit easier to assess how good these heuristics are and how much we're potentially overfitting.
Current Limitations: